Fix invalid warning when $ROS_AUTOMATIC_DISCOVERY_RANGE is not defined #343
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
PR missing one of the required labels: {'documentation', 'breaking-change', 'new feature', 'dependencies', 'bug', 'internal', 'enhancement'} |
evshary
reviewed
Nov 27, 2024
| Ok("SYSTEM_DEFAULT") => Some(RosAutomaticDiscoveryRange::SystemDefault), | ||
| Ok(value) => { | ||
| warn!( | ||
| r#"Invalid value for environment variable ROS_AUTOMATIC_DISCOVERY_RANGE ("{value}"). Using "SUBNET" instead "# |
There was a problem hiding this comment.
I'm not sure whether we should have a warning here
According to here, we shouldn't change any discovery setting. I use Subnet because I think our default configuration should be Subnet.
WDYT?
There was a problem hiding this comment.
According to here, we shouldn't change any discovery setting
Reading this doc, the case of an invalid string is not specified. So we have 2 options:
- either panic, as we can't return an Error in this
default_automatic_discovery_range()function - either ignoring the invalid value and use the default one which is
Subnetaccording to the doc. I think it's also good to log a warning in this case to point the invalid value to the user.
There was a problem hiding this comment.
Never mind. I missed the line Ok(value)
evshary
approved these changes
Nov 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Following #311 , on Humble the following warning is logged when
$ROS_AUTOMATIC_DISCOVERY_RANGEenvironment variable is not set:WARN tokio-runtime-worker ThreadId(09) zenoh_plugin_ros2dds: ROS_AUTOMATIC_DISCOVERY_RANGE will be ignored since it's not supported before ROS 2 IronThe cause is Config::default_automatic_discovery_range() setting the value to
Some(RosAutomaticDiscoveryRange::Subnet)if the environment variable is not defined.This PR fixes this.